win7: load synch functions on demand #143598
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use |
rustbot
added
O-windows
|
|
||
| pub(in crate::sys) static PTR: Atomic<*mut c_void> = AtomicPtr::new(ptr::null_mut()); | ||
| const NOT_FOUND: *mut c_void = ptr::null_mut(); | ||
| const NOT_LOADED: *mut c_void = ptr::without_provenance_mut(usize::MAX); |
There was a problem hiding this comment.
Just checking my understanding -- this is assuming (reasonably) that there's no function pointer starting at usize::MAX?
There was a problem hiding this comment.
Yes, I believe that would be impossible. On Windows at least a high bit on the would indicate a kernel pointer (which is unusable from user space), so it's outside of addressable memory. An alternative would be to use a low value (like, say, 1 or 2) because the first page is reserved as a guard against null pointer access.
| const NOT_FOUND: *mut c_void = ptr::null_mut(); | ||
| const NOT_LOADED: *mut c_void = ptr::without_provenance_mut(usize::MAX); | ||
|
|
||
| pub(in crate::sys) static PTR: Atomic<*mut c_void> = AtomicPtr::new(NOT_LOADED); |
There was a problem hiding this comment.
This makes me wonder if we should implement AtomicPrimitive for all F: FnPtr(...) at some point. Though maybe that would hurt our ability to support a generalized "fits in u8/u32/u64 without padding" Atomic...
There was a problem hiding this comment.
I would love that! Assuming it's possible.
| match PTR.load(Ordering::Relaxed) { | ||
| NOT_FOUND => None, | ||
| NOT_LOADED => load_from_module(unsafe { Module::new($module) }), | ||
| f => Some(unsafe { mem::transmute(f) }) |
There was a problem hiding this comment.
nit: could you add a safety comment on the transmutes? (In particular describing why the Option transmute is OK)
| unsafe { | ||
| static SYMBOL_NAME: &CStr = ansi_str!(sym $symbol); | ||
| if let Some(f) = module.and_then(|m| m.proc_address(SYMBOL_NAME)) { | ||
| PTR.store(f.as_ptr(), Ordering::Relaxed); |
There was a problem hiding this comment.
I guess the thinking is it's OK to have multiple GetProcAddress calls if many threads race on the initialization?
https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getprocaddress doesn't note any synchronization requirements. Should we be worried about any kind of dynamic library loading during the GetProcAddress call that might need a release/acquire if it's called from some other thread (currently not added since we're Relaxed)? I guess the load itself is happening earlier when the module is initialized (Module::new below?).
There was a problem hiding this comment.
Yeah, GetModuleHandle does the loading. GetProcAddress just reads the address from a lookup table afterwards.
In any case, all dynamic library loading takes a loader lock that synchronises the whole operation. So everything is loaded sequentially. It's a much stronger barrier than any atomic so we don't really need any cross-thread synchronisation ourselves, we just have to make sure our own read/writes are atomic.
Currently, for Windows 7 only, we do a fun thing with pre-main initialisers to optimistically load some functions (used for
park/unpark) which require Windows 8+. This means they're loaded even if unused and can mildly affect start up times (albeit not by much). More of an issue is that someone else's pre-main initialiser could run before our own (we've taken steps to mitigated that but it is just a mitigation). Also the code isn't tested by us (i.e. rust CI) any more so it's prone to bit rot. In any case, failure may not be noticed because if it does fail then the Windows 7 code will be used instead.This PR changes it to load on demand instead. In the happy case this just means doing one load and two equality checks. In the sad case where we've not yet attempted to load the module, this will be slower. But it's a one time cost, usually taken in
park(which is "slow" on account of waiting an indefinite period of time).We could alternatively just use the Windows 7 synchronisation functions exclusively. The issue with that is they're undocumented and technically could be changed or removed at any point, albeit compatibility guarantees make this somewhat unlikely. Still, we should try to be well behaved.